feat: migration dependency visualization using GraphViz/Mermaid - #643
feat: migration dependency visualization using GraphViz/Mermaid#643ElijahAhianyo wants to merge 10 commits into
Conversation
|
Bencher Report| Project | cot |
| Branch | elijah/migration-graph |
| Testbed | github-ubuntu-latest |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result microseconds (µs) (Result Δ%) | Upper Boundary microseconds (µs) (Limit %) |
|---|---|---|---|
| empty_router/empty_router | 📈 view plot 🚷 view threshold | 13,985.00 µs(+49.59%)Baseline: 9,348.94 µs | 17,769.67 µs (78.70%) |
| json_api/json_api | 📈 view plot 🚷 view threshold | 1,139.90 µs(+10.57%)Baseline: 1,030.90 µs | 1,365.87 µs (83.46%) |
| nested_routers/nested_routers | 📈 view plot 🚷 view threshold | 1,044.60 µs(+8.08%)Baseline: 966.53 µs | 1,259.08 µs (82.97%) |
| single_root_route/single_root_route | 📈 view plot 🚷 view threshold | 998.76 µs(+7.38%)Baseline: 930.09 µs | 1,221.14 µs (81.79%) |
| single_root_route_burst/single_root_route_burst | 📈 view plot 🚷 view threshold | 16,557.00 µs(-1.67%)Baseline: 16,838.11 µs | 21,470.76 µs (77.11%) |
| // canvas regardless of where it's rendered. | ||
| let _ = writeln!( | ||
| out, | ||
| "%%{{init: {{'theme': 'base', 'themeVariables': {{'background': 'transparent'}}}}}}%%" |
There was a problem hiding this comment.
Setting the background to transparent, on some occasions, doesn't work well with dark mode. It sometimes leaves a white background. check example here. I think the best way to handle this is to set styles for dark mode and light mode themes and also have users specify themes in the CLI but also provide a senseble default. I didn't want to spend time doing that in this PR and its also very low priority since the point is to allow users to visualize the graph, not necessarily to render it on a page.
| /// # Errors | ||
| /// | ||
| /// Returns an error if the dependency graph cannot be generated | ||
| pub fn to_graph(&self, format: GraphFormat) -> Result<String> { |
There was a problem hiding this comment.
I'm not quite sure if we should expose this as a public API. Public API means it's difficult to change/remove, and I believe it's not a functionality that would be particularly useful outside of the CLI. What's even more important, this functionality lies completely outside of the "MigrationEngine" scope. The role of this struct is to handle applying and rolling back migrations in a safe manner. Rendering a graph is pretty much unrelated to that.
I think we should remove this from MigrationEngine and instead it should just be a separate feature accessible only through the CLI subcommand. We'll probably need to add a getter that will return the migrations slice in the MigrationEngine.
There was a problem hiding this comment.
yeah I completely agree. should've done a better job with the abstraction
There was a problem hiding this comment.
I've moved this into a GraphExporter struct now
m4tx
left a comment
m4tx
left a comment
There was a problem hiding this comment.
A few minor issues, but now I think the structure looks good!
| use crate::utils::graph::Graph; | ||
|
|
||
| pub(super) fn render(nodes: &[Node<'_>], graph: &Graph) -> String { | ||
| #[allow(clippy::allow_attributes, clippy::wildcard_imports)] |
There was a problem hiding this comment.
Could we just use #[expect] here instead of #[allow]?
| #[allow(clippy::allow_attributes, clippy::wildcard_imports)] | |
| #[expect(clippy::wildcard_imports)] |
There was a problem hiding this comment.
using expect here doesnt work. I typically get this warning with clippy:
Checking cot-test v0.1.0 (/Users/eli/Documents/work/cot/cot/cot-test)
Checking example-todo-list v0.1.0 (/Users/eli/Documents/work/cot/cot/examples/todo-list)
warning: this lint expectation is unfulfilled
--> cot/src/db/migrations/graph_export/dot.rs:7:14
|
7 | #[expect(clippy::wildcard_imports)]
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(unfulfilled_lint_expectations)]` on by default
warning: this lint expectation is unfulfilled
--> cot/src/db/migrations/graph_export/mermaid.rs:7:14
|
7 | #[expect(clippy::wildcard_imports)]
| ^^^^^^^^^^^^^^^^^^^^^^^^
warning: `cot` (lib test) generated 2 warnings
Finished `dev` profile [unoptimized + debuginfo] target(s) in 13.32s
After some digging, I found that Clippy has a special case for wildcards where the lint fires when the binary is compiled normally (in non-test mode), but it is entirely skipped in test mode, which means in test mode, the expect line becomes false. More on that here
Perhaps we should maintain the use of allow here and specify a reason linking to the Clippy issue?
| } | ||
|
|
||
| fn wrap_label(label: &str) -> Vec<String> { | ||
| #[allow(clippy::allow_attributes, clippy::wildcard_imports)] |
There was a problem hiding this comment.
Same here (#[expect] instead of allow).
m4tx
changed the title
2 participants
Description
Add graph subcommand to the migration subcommand to generate graphs in Graphiz dot format and mermaidjs.
Example usage
By default, this uses the dot format and prints to stdout.
This is analogous to :
> cargo run migration graph --format dotYou can also specify an output file to write the output to :
Type of change